Skip to content

fix(animated): prevent negative listener count in AnimatedValue#2

Open
mhdamirhamza wants to merge 10 commits into
mainfrom
mhdamirhamza-patch-2
Open

fix(animated): prevent negative listener count in AnimatedValue#2
mhdamirhamza wants to merge 10 commits into
mainfrom
mhdamirhamza-patch-2

Conversation

@mhdamirhamza

Copy link
Copy Markdown
Owner

Fixed an issue in AnimatedValue.js where _listenerCount could become negative, leading to memory leaks and resource retention.
​Problem:
If removeListener is called more times than addListener (e.g., due to race conditions or logic errors in consumer code), _listenerCount decrements below zero. This causes the cleanup logic (this._updateSubscription?.remove()) to never execute, leaking native subscriptions.
​Fix:
Used Math.max(0, this._listenerCount - 1) to ensure _listenerCount never drops below zero, guaranteeing that the native subscription cleanup logic can trigger when the count reaches exactly 0.

Fixed an issue in AnimatedValue.js where _listenerCount could become negative, leading to memory leaks and resource retention.
​Problem:
If removeListener is called more times than addListener (e.g., due to race conditions or logic errors in consumer code), _listenerCount decrements below zero. This causes the cleanup logic (this._updateSubscription?.remove()) to never execute, leaking native subscriptions.
​Fix:
Used Math.max(0, this._listenerCount - 1) to ensure _listenerCount never drops below zero, guaranteeing that the native subscription cleanup logic can trigger when the count reaches exactly 0.
Added AnimatedColor-test.js to cover the negative listener count bug after __detach and removeListener, as requested by @javache.
Overrode addListener, removeListener, and removeAllListeners to mirror AnimatedValueXY architecture and utilize a local listeners map. This handles edge cases like calling removeListener after __detach seamlessly.
## Test Plan
Added a comprehensive regression test suite in `packages/react-native/Libraries/Animated/tests/AnimatedColor-test.js` covering:
- Verifying `_listenerCount` does not drop into negative values after a `__detach()` and subsequent `removeListener()` execution.
- Testing `removeListener` safely acts as a no-op when called after `removeAllListeners()`.
- Ensuring native driver subscriptions are properly initiated and systematically torn down without memory leaks during complex component unmount/re-attach lifecycles.
Tested locally by running:
`yarn jest AnimatedColor-test`

## Changelog
[General] [Fixed] - Prevent negative listener count and native subscription memory leaks in AnimatedColor during detach cascades.
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. -->

## Summary:
Fixed an issue in `AnimatedColor.js` where `_listenerCount` could become negative during complex unmount/detach phases. When `__detach()` is invoked on `AnimatedColor`, it triggers `removeAllListeners()` on its underlying channel nodes (r, g, b, a), resetting their counts to 0. Subsequent stale infrastructure cleanups safely invoke `removeListener()`, which blindly decremented the counter to -1, bypassing the `=== 0` check and permanently leaking native subscriptions. 

This PR implements an overridden `removeListener` and `addListener` routine in `AnimatedColor` (mirroring the robust pattern in `AnimatedValueXY`) to act as a defensive shield and guarantee stable listener lifecycles.

## Changelog:
[General] [Fixed] - Prevent negative listener count and native subscription memory leaks in AnimatedColor during detach cascades

## Test Plan:
Added a comprehensive regression test suite in `packages/react-native/Libraries/Animated/tests/AnimatedColor-test.js` covering:
- Verifying `_listenerCount` does not drop into negative values after a `__detach()` and subsequent `removeListener()` execution.
- Testing `removeListener` safely acts as a no-op when called after `removeAllListeners()`.
- Ensuring native driver subscriptions are properly initiated and systematically torn down without memory leaks during complex component unmount/re-attach lifecycles.

Tested locally by running:
`yarn jest AnimatedColor-test`
## Summary:

Fixed an issue in `AnimatedColor.js` where `_listenerCount` could become negative during component unmount, leading to native subscription memory leaks.

When `AnimatedColor.__detach()` is called, it invokes `removeAllListeners()` on its channel nodes (r, g, b, a), resetting their `_listenerCount` to 0. If infrastructure cleanup then calls `removeListener(id)` on those same nodes, the count drops to -1, permanently bypassing the cleanup condition `if (this._listenerCount === 0)` — so `this._updateSubscription?.remove()` never fires.

Fix: Added a `_listeners` map to `AnimatedColor` with overrides for `addListener`, `removeListener`, and `removeAllListeners`. `removeListener` now returns early if the id is not found (already cleaned up by `__detach`), mirroring the pattern used by `AnimatedValueXY`.

## Changelog:

[GENERAL] [FIXED] - Prevent negative listener count in AnimatedColor causing native subscription memory leaks after `__detach()`

## Test Plan:

Added regression tests in `Libraries/Animated/tests/AnimatedColor-test.js`:

- `_listenerCount` does not go negative after `__detach()` + `removeListener()`
- `removeListener` after `removeAllListeners` is a safe no-op
- Native subscription starts correctly for all 4 channels (r/g/b/a)
- Native subscription cleans up correctly when count reaches 0
- No native subscription leak after re-attach cycle
## Summary:

Fixed an issue in AnimatedColor.js where _listenerCount could become 
negative during unmount, causing native subscription memory leaks...

## Changelog:

[GENERAL] [FIXED] - Prevent negative listener count in AnimatedColor 
causing native subscription memory leaks after __detach()

## Test Plan:

Added regression tests in AnimatedColor-test.js covering:
- _listenerCount does not go negative after __detach() + removeListener()
- removeListener after removeAllListeners is a safe no-op
- Native subscription cleanup when count reaches 0
## Summary:
Fixed an issue in AnimatedColor.js where _listenerCount could become negative during complex unmount phases. When __detach() is executed, it invokes removeAllListeners() on underlying channel nodes (r, g, b, a), resetting counts to 0. Subsequent stale infrastructure cleanups invoke removeListener(), which blindly decrements the counter to -1, bypassing the === 0 check and leaking native subscriptions permanently.

## Changelog:
[General] [Fixed] - Prevent negative listener count and native subscription memory leaks in AnimatedColor during detach cascades

## Test Plan:
Added a comprehensive regression test suite in packages/react-native/Libraries/Animated/tests/AnimatedColor-test.js covering:
- Verifying _listenerCount does not drop into negative values after a __detach() and subsequent removeListener() execution.
- Testing removeListener safely acts as a no-op when called after removeAllListeners().
- Ensuring native driver subscriptions are properly initiated and systematically torn down without memory leaks.

Tested locally by running:
yarn jest AnimatedColor-test
## Summary:
Fixed negative listener count bug in AnimatedColor causing memory leaks.

## Changelog:
[GENERAL] [FIXED] - Prevent negative listener count in AnimatedColor after __detach()

## Test Plan:
Added regression tests in AnimatedColor-test.js covering listener count and cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant